feat: hmac authentication strategy and response verification#8262
feat: hmac authentication strategy and response verification#8262bitgoAaron wants to merge 1 commit intomasterfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
bitgoAaron
force-pushed
the
aloe/hmacWebCryptoStrategy
branch
2 times, most recently
from
729a786 to
0cbe167
Compare
- Updated function to be asynchronous, allowing for better handling of HMAC verification. - Introduced for default HMAC handling and added support for custom strategies. - Integrated for browser compatibility, enabling HMAC signing and verification using the Web Crypto API. - Enhanced to utilize the new HMAC strategies for request signing and response verification. - Added unit tests for the new HMAC strategies and their integration with the BitGoAPI. - Updated web demo to include a new component for WebCrypto authentication. Ticket: CE-10122
bitgoAaron
force-pushed
the
aloe/hmacWebCryptoStrategy
branch
from
0cbe167 to
49529e2
Compare
| appendLog('No stored CryptoSigning found in IndexedDB.'); | ||
| } | ||
|
|
||
| const options: Record<string, unknown> = { |
There was a problem hiding this comment.
| const options: Record<string, unknown> = { | |
| const options: BitGoAPIOptions = { |
I know this is a demo app, but might be worth using a type safe param here so if there's a breaking change, we'd get type errors as another signal something broke
| params.timestamp >= now - backwardValidityWindow && params.timestamp <= now + forwardValidityWindow; | ||
|
|
||
| return { | ||
| isValid: expectedHmac === params.hmac, |
There was a problem hiding this comment.
In hmacv4.ts we use crypto.timingSafeEqual. While this is unlikely to be an issue, probably best to use that here too for timing attack prevention. Might need a polyfill for this though.
| const queryPath = extractQueryPath(params.urlPath); | ||
|
|
||
| let prefixedText: string; | ||
| if (params.statusCode !== undefined && isFinite(params.statusCode) && Number.isInteger(params.statusCode)) { |
There was a problem hiding this comment.
Linting suggests Number.isFinite instead of isFinite
| VerifyResponseInfo, | ||
| } from './types'; | ||
|
|
||
| function arrayBufToHex(buffer: ArrayBuffer): string { |
There was a problem hiding this comment.
This is unbounded, might it be worth putting a max size on this?
Also in Node.js you could do (about 100x faster):
function arrayBufToHexNode(buffer: ArrayBuffer): string {
return Buffer.from(buffer).toString('hex');
}Maybe could have different implementations/code paths for browser and Node.js?
I ran a quick benchmark with new ArrayBuffer(1024) // 1KB:
arrayBufToHex: 0.155ms
arrayBufToHexNode: 0.02ms
and new ArrayBuffer(1024 * 1024 * 10) // 10MB:
arrayBufToHex: 487.858ms
arrayBufToHexNode: 4.55ms
| const expectedHmac = await webCryptoHmacSign(this.cryptoKey, subject); | ||
|
|
||
| const now = Date.now(); | ||
| const backwardValidityWindow = 1000 * 60 * 5; |
There was a problem hiding this comment.
Any reason for 5 minutes instead of just 1 minute? Not sure how long our timeouts are, but iirc HMAC is done right at the end, just before the request is sent down the wire, so we should only really need to take network latency and user clock drift into account plus some buffer
| const bytes = new Uint8Array(buffer); | ||
| const hexParts: string[] = new Array(bytes.length); | ||
| for (let i = 0; i < bytes.length; i++) { | ||
| hexParts[i] = bytes[i].toString(16).padStart(2, '0'); |
There was a problem hiding this comment.
A lookup table would be quicker for buffers greater than a few hundred bytes
const hexTable = Array.from({ length: 256 }, (_, i) => i.toString(16).padStart(2, '0'));
for (let i = 0; i < bytes.length; i++) {
hexChars.push(hexTable[bytes[i]]);
}2 participants
Ticket: CE-10122